Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent crash when starting DynamoSandbox without ASM #9210

Merged
merged 4 commits into from
Dec 5, 2018
Merged

Prevent crash when starting DynamoSandbox without ASM #9210

merged 4 commits into from
Dec 5, 2018

Conversation

Dewb
Copy link
Contributor

@Dewb Dewb commented Oct 26, 2018 •
edited
Loading

Purpose

Prevent a crash when starting DynamoSandbox on a system where ASM is not (or cannot be) found.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • (n/a) User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • (n/a) Snapshot of UI changes, if any.
  • (n/a) Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

@Dewb Dewb requested a review from mjkkirschner October 26, 2018 22:52
mjkkirschner
mjkkirschner approved these changes Oct 27, 2018
View reviewed changes
Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dewb thanks for looking into this!
LGTM - potentially we can cherry pick this to 2.0.2 branch as that branch also has these changes around libG loading.

@mjkkirschner
Copy link
Member

we can also add a test like this to the libGPreloaderTests.cs file

    [Test]
        public void PreloaderThatDoesNotFindASMDoesNotThrow()
        {
            Assert.DoesNotThrow(() =>
            {
                var preloader = new Preloader(Path.GetTempPath(), new[] { new Version(999, 999, 999) });
            });
        }

I've verified this would fail before your fix.

hauswij
hauswij reviewed Nov 15, 2018
View reviewed changes
src/Tools/DynamoShapeManager/Preloader.cs Outdated
var libGFolderName = "";
if (version != null)
{
libGFolderName = string.Format("libg_{0}_{1}_{2}", version.Major, version.Minor, version.Build);
Copy link

@hauswij hauswij Nov 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If version is null, then libGFolderName won't be correct (empty). And then preloaderLocation/geometryFactoryPath will not be pointing to any dlls, but they will not be empty either. Worse yet, geometryFactoryPath will not even be a correct path.
I think it would be better that preloaderLocation/geometryFactoryPath be empty so that you can later check if they're not empty.

Suggested change
libGFolderName = string.Format("libg_{0}_{1}_{2}", version.Major, version.Minor, version.Build);
if (version == null)
{
return;
}
var libGFolderName = string.Format("libg_{0}_{1}_{2}", version.Major, version.Minor, version.Build);

Just returning will/should keep preloaderLocation/geometryFactoryPath not set/empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a good point. I'm not sure what the consequences of leaving these paths empty might be. The intent of this change is just to fix the crash, so preserving the earlier behavior of setting up valid but nonexistent libg_0 path seems the safest route. Additional work in this area would definitely be warranted.

@Dewb
Copy link
Contributor Author

Dewb commented Nov 20, 2018

Made a change in response to @hauswij's comments, and added @mjkkirschner's test case.

@QilongTang
Copy link
Contributor

@Dewb @mjkkirschner @Racel @aparajit-pratap Not trying to revise what we commit to do here. I am reviewing how this case has been working in history. I do recall it does not crash in Dynamo 1.X when I joined the team. And in my home machine, I could not reproduce the crash after uninstalling Revit. I was able to launch DynamoSandbox in 2.0.1 ( last release ) and when placing a node producing geometry, as a user I would get a warning about LibG:
image

Do you guys recall the behavior in previous versions?

@aparajit-pratap
Copy link
Contributor

@QilongTang I do not remember Dynamo starting in the absence of ASM. In any case I tried running Dynamo sandbox 1.3.4 without ASM and it does not (crashes).

@mjkkirschner
Copy link
Member

@QilongTang I believe there has never before been a test for this functionality - so it's gone back and forth, I believe I broke it in 2.0.2/master with the work for loading different versions of ASM with the same major version.

@QilongTang
Copy link
Contributor

@mjkkirschner That makes sense. I was only curious because to me it works in 2.0.1. @Dewb We will test shortly and let you know

@aparajit-pratap
Copy link
Contributor

@mjkkirschner @QilongTang I'm confused. Are you sure you can startup sandbox without ASM in 2.0?

@QilongTang
Copy link
Contributor

@aparajit-pratap I am sure for 2.0.1, not sure for 1.3

@QilongTang
Copy link
Contributor

Merging this and ready to test in the coming sprint

@QilongTang QilongTang merged commit 06ee93f into DynamoDS:master Dec 5, 2018
@horatiubota horatiubota added the error/warning/crash Issues mentioning a Dynamo error, warning or crash message label Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hauswij hauswij hauswij left review comments

@mjkkirschner mjkkirschner mjkkirschner approved these changes

Assignees
No one assigned
Labels
error/warning/crash Issues mentioning a Dynamo error, warning or crash message
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Dewb @mjkkirschner @QilongTang @aparajit-pratap @hauswij @horatiubota @mike-d-adsk